Refactor FederatedCredentials e2e test to use stable repository with config-based disabling#2104
Refactor FederatedCredentials e2e test to use stable repository with config-based disabling#2104
Conversation
…repository Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
…rove pagination and timestamp comparison Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
…federated credentials Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
…function, exclude tmp-bingmaps from cleanup Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
Done. Updated |
…ce to avoid auto-cleanup Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
…reason in each object Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
…ontext not available at job level) Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
Co-authored-by: mazhelez <43066499+mazhelez@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the FederatedCredentials end-to-end test to maintain a stable repository identity, which is required for federated credential authentication that no longer supports repository name-based matching. The test now reuses a pre-existing repository (e2e-bingmaps.appsource) instead of deleting and recreating it on each run. Additionally, a config-based mechanism for disabling scenarios is introduced.
Changes:
- Renamed test repository from
tmp-bingmaps.appsourcetoe2e-bingmaps.appsourceand requires manual pre-creation with federated credentials configured - Added helper functions
CleanupWorkflowRunsandResetRepositoryToSourceto maintain deterministic test state - Implemented config-based scenario disabling via
e2eTests/disabled-scenarios.jsonwith comprehensive workflow logging
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| e2eTests/scenarios/FederatedCredentials/runtest.ps1 | Refactored to reuse stable repository, reset to source state, clean workflow runs, and use timestamp-based workflow tracking |
| e2eTests/e2eTestHelper.psm1 | Added CleanupWorkflowRuns and ResetRepositoryToSource helper functions with PowerShell comment-based documentation |
| e2eTests/disabled-scenarios.json | New config file defining disabled scenarios with reasons (FederatedCredentials currently disabled) |
| .github/workflows/E2E.yaml | Loads disabled scenarios from config and filters them from test matrix with comprehensive logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function CleanupWorkflowRuns { | ||
| Param( | ||
| [Parameter(Mandatory = $true)] | ||
| [string] $repository | ||
| ) | ||
|
|
||
| Write-Host -ForegroundColor Yellow "`nCleaning up workflow runs in $repository" | ||
|
|
||
| RefreshToken -repository $repository | ||
|
|
||
| # Get all workflow runs | ||
| $runs = invoke-gh api "/repos/$repository/actions/runs?per_page=100" -silent -returnValue | ConvertFrom-Json | ||
|
|
||
| if ($runs.workflow_runs.Count -eq 0) { | ||
| Write-Host "No workflow runs found" | ||
| return | ||
| } | ||
|
|
||
| Write-Host "Deleting $($runs.workflow_runs.Count) workflow runs..." | ||
| foreach ($run in $runs.workflow_runs) { | ||
| try { | ||
| Write-Host "Deleting run $($run.id) ($($run.name) - $($run.status))" | ||
| invoke-gh api /repos/$repository/actions/runs/$($run.id) --method DELETE -silent | Out-Null | ||
| } | ||
| catch { | ||
| Write-Host "Warning: Failed to delete run $($run.id): $_" | ||
| } | ||
| } | ||
| Write-Host "Cleanup completed" |
There was a problem hiding this comment.
The function only retrieves the first 100 workflow runs (per_page=100) and doesn't handle pagination. If a repository has more than 100 workflow runs, some will not be cleaned up. Consider adding pagination logic to iterate through all pages, or document this limitation in the function's documentation.
| $updateCreatedAt = [DateTime]$updateRun.created_at | ||
| $runs = invoke-gh api /repos/$repository/actions/runs -silent -returnValue | ConvertFrom-Json | ||
| $run = $runs.workflow_runs | Select-Object -First 1 | ||
|
|
||
| # Find the CI/CD workflow run that started after the update workflow was created | ||
| $run = $runs.workflow_runs | Where-Object { | ||
| $_.event -eq 'push' -and [DateTime]$_.created_at -gt $updateCreatedAt |
There was a problem hiding this comment.
Using created_at from the update workflow run to find subsequent CI/CD runs may be imprecise. Since the Update AL-Go System Files workflow runs with -wait (line 117), it completes before returning. The CI/CD workflow is triggered by the push event when the update workflow completes, so comparing against the update workflow's created_at (when it started) rather than when it completed could cause issues if multiple workflows run concurrently. Consider using $updateRun.updated_at or adding a small time buffer to ensure we capture the CI/CD run triggered by the update workflow's completion.
| $beforeFilter = $filteredScenarios.Count | ||
| $filteredScenarios = $filteredScenarios | Where-Object { $disabledScenarios -notcontains $_ } | ||
| $afterFilter = $filteredScenarios.Count | ||
| if ($beforeFilter -ne $afterFilter) { | ||
| Write-Host "Filtered out $($beforeFilter - $afterFilter) disabled scenario(s)" | ||
| $disabledScenariosConfig | Where-Object { $filteredScenarios -notcontains $_.scenario } | ForEach-Object { |
There was a problem hiding this comment.
The logic for logging filtered scenarios may show incorrect results. The condition $filteredScenarios -notcontains $_.scenario will match all disabled scenarios from the config that are not in the final filtered list, but this includes disabled scenarios that were never in the list to begin with (e.g., if they didn't match the scenariosFilter). To accurately show only the scenarios that were actually filtered out, you should capture the pre-filter list and check: ($scenariosBeforeDisabledFilter -contains $_.scenario) -and ($filteredScenarios -notcontains $_.scenario)
| $beforeFilter = $filteredScenarios.Count | |
| $filteredScenarios = $filteredScenarios | Where-Object { $disabledScenarios -notcontains $_ } | |
| $afterFilter = $filteredScenarios.Count | |
| if ($beforeFilter -ne $afterFilter) { | |
| Write-Host "Filtered out $($beforeFilter - $afterFilter) disabled scenario(s)" | |
| $disabledScenariosConfig | Where-Object { $filteredScenarios -notcontains $_.scenario } | ForEach-Object { | |
| $scenariosBeforeDisabledFilter = $filteredScenarios | |
| $beforeFilter = $filteredScenarios.Count | |
| $filteredScenarios = $filteredScenarios | Where-Object { $disabledScenarios -notcontains $_ } | |
| $afterFilter = $filteredScenarios.Count | |
| if ($beforeFilter -ne $afterFilter) { | |
| Write-Host "Filtered out $($beforeFilter - $afterFilter) disabled scenario(s)" | |
| $disabledScenariosConfig | Where-Object { ($scenariosBeforeDisabledFilter -contains $_.scenario) -and ($filteredScenarios -notcontains $_.scenario) } | ForEach-Object { |
| # The CI/CD workflow should fail because the version number of the app in thie repository is lower than the version number in AppSource | ||
| # The CI/CD workflow should fail because the version number of the app in the repository is lower than the version number in AppSource | ||
| # Reason being that major.minor from the original bcsamples-bingmaps.appsource is the same and the build number in the newly created repository is lower than the one in AppSource | ||
| # This error is expected we will grab the version number from AppSource, add one to revision number (by switching to versioningstrategy 3 in the tmp repo) and use it in the next run |
There was a problem hiding this comment.
The comment references "tmp repo" but the repository has been renamed to "e2e-bingmaps.appsource". This should be updated to reflect the new naming.
| # This error is expected we will grab the version number from AppSource, add one to revision number (by switching to versioningstrategy 3 in the tmp repo) and use it in the next run | |
| # This error is expected we will grab the version number from AppSource, add one to revision number (by switching to versioningstrategy 3 in the e2e-bingmaps.appsource repo) and use it in the next run |
| # Load disabled scenarios from config file | ||
| $disabledScenariosConfig = Get-Content -path (Join-Path $ENV:GITHUB_WORKSPACE "e2eTests/disabled-scenarios.json") -encoding UTF8 -raw | ConvertFrom-Json |
There was a problem hiding this comment.
The code doesn't handle the case where the disabled-scenarios.json file doesn't exist. If the file is missing, the Get-Content command will throw an error and fail the workflow. Consider adding error handling or a check for file existence, or document that the file must always exist (even if empty as []).
| # Load disabled scenarios from config file | |
| $disabledScenariosConfig = Get-Content -path (Join-Path $ENV:GITHUB_WORKSPACE "e2eTests/disabled-scenarios.json") -encoding UTF8 -raw | ConvertFrom-Json | |
| # Load disabled scenarios from config file (optional) | |
| $disabledScenariosConfigPath = Join-Path $ENV:GITHUB_WORKSPACE "e2eTests/disabled-scenarios.json" | |
| $disabledScenariosConfig = @() | |
| if (Test-Path -Path $disabledScenariosConfigPath) { | |
| $disabledScenariosContent = Get-Content -Path $disabledScenariosConfigPath -Encoding UTF8 -Raw | |
| if (-not [string]::IsNullOrWhiteSpace($disabledScenariosContent)) { | |
| $disabledScenariosConfig = $disabledScenariosContent | ConvertFrom-Json | |
| } | |
| } | |
| else { | |
| Write-Host "No disabled-scenarios.json found; proceeding with all scenarios enabled." | |
| } |
| # Fallback to the first workflow run if we can't find one based on timestamp | ||
| Write-Host "Warning: Could not find CI/CD run based on timestamp, using first run" | ||
| $run = $runs.workflow_runs | Select-Object -First 1 |
There was a problem hiding this comment.
The fallback logic uses Select-Object -First 1 on all workflow runs without filtering by event type. This could select a workflow run that's not a push event (e.g., a workflow_dispatch event), which wouldn't be the CI/CD run triggered by the Update AL-Go System Files workflow. Consider filtering by event type in the fallback: $run = $runs.workflow_runs | Where-Object { $_.event -eq 'push' } | Select-Object -First 1
| # Fallback to the first workflow run if we can't find one based on timestamp | |
| Write-Host "Warning: Could not find CI/CD run based on timestamp, using first run" | |
| $run = $runs.workflow_runs | Select-Object -First 1 | |
| # Fallback to the first push workflow run if we can't find one based on timestamp | |
| Write-Host "Warning: Could not find CI/CD run based on timestamp, using first push run" | |
| $run = $runs.workflow_runs | Where-Object { $_.event -eq 'push' } | Select-Object -First 1 |
| gh api repos/$repository --method HEAD | ||
| if ($LASTEXITCODE -eq 0) { | ||
| Write-Host "Repository $repository already exists. Deleting it." | ||
| gh repo delete $repository --yes | Out-Host | ||
| Start-Sleep -Seconds 30 | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Repository $repository does not exist. The repository must be created manually with federated credentials configured before running this test." | ||
| } |
There was a problem hiding this comment.
Using gh api directly instead of invoke-gh is inconsistent with the rest of the codebase. The invoke-gh wrapper provides consistent error handling and authentication setup. This direct call should be replaced with invoke-gh wrapped in a try-catch block to handle the case where the repository doesn't exist.
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to clone repository $repository" | ||
| } | ||
|
|
||
| # Fetch the source repository content | ||
| Write-Host "Fetching source repository $sourceRepository..." | ||
| invoke-git remote add source "https://github.com/$sourceRepository.git" | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to add remote source for $sourceRepository" | ||
| } | ||
|
|
||
| invoke-git fetch source $branch --quiet | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to fetch branch $branch from source $sourceRepository" | ||
| } | ||
|
|
||
| # Reset the current branch to match the source | ||
| Write-Host "Resetting $branch to match source/$branch..." | ||
| invoke-git checkout $branch --quiet | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to checkout branch $branch" | ||
| } | ||
|
|
||
| invoke-git reset --hard "source/$branch" --quiet | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to reset branch $branch to source/$branch" | ||
| } | ||
|
|
||
| # Force push to update the repository | ||
| Write-Host "Force pushing changes..." | ||
| invoke-git push origin $branch --force --quiet | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to push changes to $repository" | ||
| } |
There was a problem hiding this comment.
The $LASTEXITCODE checks after invoke-gh and invoke-git calls are unnecessary and incorrect. These wrapper functions use cmdDo, which throws an exception when a command returns a non-zero exit code (see Actions/Github-Helper.psm1 line 296-298). The exit code checks will never be reached because an exception will be thrown first. These checks can be removed, as the try-catch block will handle errors.
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to clone repository $repository" | |
| } | |
| # Fetch the source repository content | |
| Write-Host "Fetching source repository $sourceRepository..." | |
| invoke-git remote add source "https://github.com/$sourceRepository.git" | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to add remote source for $sourceRepository" | |
| } | |
| invoke-git fetch source $branch --quiet | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to fetch branch $branch from source $sourceRepository" | |
| } | |
| # Reset the current branch to match the source | |
| Write-Host "Resetting $branch to match source/$branch..." | |
| invoke-git checkout $branch --quiet | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to checkout branch $branch" | |
| } | |
| invoke-git reset --hard "source/$branch" --quiet | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to reset branch $branch to source/$branch" | |
| } | |
| # Force push to update the repository | |
| Write-Host "Force pushing changes..." | |
| invoke-git push origin $branch --force --quiet | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to push changes to $repository" | |
| } | |
| # Fetch the source repository content | |
| Write-Host "Fetching source repository $sourceRepository..." | |
| invoke-git remote add source "https://github.com/$sourceRepository.git" | |
| invoke-git fetch source $branch --quiet | |
| # Reset the current branch to match the source | |
| Write-Host "Resetting $branch to match source/$branch..." | |
| invoke-git checkout $branch --quiet | |
| invoke-git reset --hard "source/$branch" --quiet | |
| # Force push to update the repository | |
| Write-Host "Force pushing changes..." | |
| invoke-git push origin $branch --force --quiet |
❔What, Why & How
Federated credentials require stable repository identity. Refactored the test to reuse
e2e-bingmaps.appsourceinstead of deleting/recreatingtmp-bingmaps.appsourceon each run. Test is currently disabled via config file pending Azure resource migration.Helper functions added to
e2eTestHelper.psm1:CleanupWorkflowRuns: Deletes all workflow runs in a repository (ensures clean tracking state)ResetRepositoryToSource: Hard resets repository content to match source viagit reset --hard+ force push (preserves identity, resets content)Test flow changes in
scenarios/FederatedCredentials/runtest.ps1:tmp-bingmaps.appsource→e2e-bingmaps.appsource(avoidstmp*cleanup pattern)bcsamples-bingmaps.appsourcecontent before each runghTokenWorkflowSecretName) after resetWorkflow changes in
.github/workflows/E2E.yaml:e2eTests/disabled-scenarios.jsonconfig file to manage disabled scenarios (array format with scenario and reason in each object)Config file format (
e2eTests/disabled-scenarios.json):[ { "scenario": "FederatedCredentials", "reason": "Azure resource migration work in progress" } ]Technical details:
-- --quietflag fromgh repo cloneTo re-enable FederatedCredentials test: Remove the FederatedCredentials object from the array in
e2eTests/disabled-scenarios.json.When re-enabled, the test will use the stable repository instead of recreating it, maintaining federated credential authentication.
✅ Checklist
Original prompt
This pull request was created from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.